-
Notifications
You must be signed in to change notification settings - Fork 3
fix: use img instead of next's Image for less memory leaks #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughReplaced Next.js Image components with standard HTML img elements across demo pages and components, adjusted related classes (e.g., overflow-hidden, object-cover, w-full) and added sizing/format query parameters to image URLs in data.json. No exported APIs changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/components/Navbar.tsx (1)
92-97
: Logo img: add display sizing and eager/priority hintsSet a predictable rendered size and prioritize the above-the-fold logo for LCP; also enable async decoding.
- <img - src="/logo.svg" - alt="Request Network" - width={100} - height={200} - /> + <img + src="/logo.svg" + alt="Request Network" + width={100} + height={200} + className="h-8 w-auto" + loading="eager" + decoding="async" + fetchpriority="high" + />If the goal is to reduce memory, confirm that the rendered size matches design expectations (e.g., h-8) and doesn’t upscale the intrinsic asset.
src/components/EventShowcase.tsx (1)
72-76
: Ensure image fills its aspect-ratio container and lazily loadsWithout h-full/w-full the image may not fully cover the 16:9 box. Also add lazy loading/async decoding.
- <img - src={event.image} - alt={event.name} - className="object-cover transition-transform duration-300 group-hover:scale-105" - /> + <img + src={event.image} + alt={event.name} + className="h-full w-full object-cover transition-transform duration-300 group-hover:scale-105" + loading="lazy" + decoding="async" + />For sharper images on high-DPR screens, consider adding a srcSet with a 2x variant (e.g., increasing the w param) in a follow-up.
src/app/(demo)/page.tsx (1)
40-44
: Carousel slide image: fill container + lazy/asyncMatch the container’s aspect ratio and defer decoding.
- <img - src={event.headerImage} - alt={event.name} - className="object-cover" - /> + <img + src={event.headerImage} + alt={event.name} + className="h-full w-full object-cover" + loading="lazy" + decoding="async" + />If the first slide is above the fold, consider eager loading only for that slide (index 0) and lazy for the rest.
src/app/(demo)/event/[id]/page.tsx (2)
29-34
: Hero image: fill, eager load, and set fetch priorityBring back LCP-friendly behavior you previously got from next/image and ensure full coverage within the fixed-height container.
- <div className="relative h-[400px] w-full overflow-hidden"> - <img - src={event.headerImage} - alt={event.name} - className="object-cover w-full" - /> + <div className="relative h-[400px] w-full overflow-hidden"> + <img + src={event.headerImage} + alt={event.name} + className="h-full w-full object-cover" + loading="eager" + decoding="async" + fetchpriority="high" + sizes="100vw" + />Confirm LCP doesn’t regress compared to next/image on this route.
87-91
: Organizer avatar: fill container + lazy/asyncEnsure the avatar fully covers the circular frame and defers loading.
- <img - src={event.organizer.logo} - alt={event.organizer.name} - className="object-cover" - /> + <img + src={event.organizer.logo} + alt={event.organizer.name} + className="h-full w-full object-cover" + loading="lazy" + decoding="async" + />src/const/data.json (1)
57-59
: Optional: plan for high-DPR screensTo keep memory in check while improving sharpness on Retina, consider adding srcSet in components (not here) with a 2x variant (e.g., same URL with doubled w), letting the browser choose.
Also applies to: 106-108, 148-150, 197-199, 246-248, 288-290, 330-332, 372-374, 414-416
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/(demo)/event/[id]/page.tsx
(2 hunks)src/app/(demo)/page.tsx
(2 hunks)src/components/EventShowcase.tsx
(1 hunks)src/components/Navbar.tsx
(1 hunks)src/const/data.json
(20 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-15T11:08:35.434Z
Learnt from: aimensahnoun
PR: RequestNetwork/rn-checkout#22
File: src/app/(demo)/event/[id]/page.tsx:105-107
Timestamp: 2024-11-15T11:08:35.434Z
Learning: In the `EventDetailsPage` component in `src/app/(demo)/event/[id]/page.tsx`, adding an error boundary to the `TicketSelector` component and optimizing props by only passing necessary event data is not required.
Applied to files:
src/components/EventShowcase.tsx
src/app/(demo)/page.tsx
src/app/(demo)/event/[id]/page.tsx
📚 Learning: 2024-11-15T11:05:58.024Z
Learnt from: aimensahnoun
PR: RequestNetwork/rn-checkout#22
File: src/app/(demo)/page.tsx:26-26
Timestamp: 2024-11-15T11:05:58.024Z
Learning: In `src/app/(demo)/page.tsx`, when reviewing the carousel component, it's not necessary to add accessibility controls such as `pauseOnHover` and adjusting the `interval`.
Applied to files:
src/app/(demo)/page.tsx
src/app/(demo)/event/[id]/page.tsx
📚 Learning: 2024-11-15T11:08:56.671Z
Learnt from: aimensahnoun
PR: RequestNetwork/rn-checkout#22
File: src/app/(demo)/event/[id]/page.tsx:8-13
Timestamp: 2024-11-15T11:08:56.671Z
Learning: In `src/app/(demo)/event/[id]/page.tsx`, the `getEventById` function always receives a valid, hardcoded ID. Therefore, checking for undefined or null IDs is unnecessary in this context.
Applied to files:
src/app/(demo)/event/[id]/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install Dependencies
🔇 Additional comments (1)
src/const/data.json (1)
8-10
: LGTM on right-sizing Unsplash URLsw/q/auto=format are good defaults and align with the img migration.
Spot-check that 1200px header images aren’t upscaled on large displays; consider increasing w for hero contexts if they appear blurry.
e2d8722
to
16632f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/(demo)/event/[id]/page.tsx
(2 hunks)src/app/(demo)/page.tsx
(1 hunks)src/components/EventShowcase.tsx
(1 hunks)src/components/Navbar.tsx
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/(demo)/page.tsx
- src/app/(demo)/event/[id]/page.tsx
- src/components/Navbar.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-15T11:08:35.434Z
Learnt from: aimensahnoun
PR: RequestNetwork/rn-checkout#22
File: src/app/(demo)/event/[id]/page.tsx:105-107
Timestamp: 2024-11-15T11:08:35.434Z
Learning: In the `EventDetailsPage` component in `src/app/(demo)/event/[id]/page.tsx`, adding an error boundary to the `TicketSelector` component and optimizing props by only passing necessary event data is not required.
Applied to files:
src/components/EventShowcase.tsx
Problem
The checkout page is leaking memory in production. We are suspecting NextJS's
Image
component.Solution
After running some automated stress tests for half an hour on my local build, the results are something like:
Image
component - 149MB (+87) heap sizeimg
component - 126MB (+64) heap sizeAdditionally optimized the images by modifying the URLs to serve us lower quality pictures.
It may not completely solve the leak, but it's worth a shot. Other search results pointed to some NextJS and Docker specific issues, let's hope it's not that.
Summary by CodeRabbit